Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Toolbar Simplification #6290

Merged
merged 2 commits into from
Oct 12, 2019
Merged

Toolbar Simplification #6290

merged 2 commits into from
Oct 12, 2019

Conversation

epwinchell
Copy link
Contributor

@epwinchell epwinchell commented Oct 9, 2019 •
edited
Loading

This PR removes all top level icons when text is also present, with the exception of custom button groups.

Old
Screen Shot 2019-10-09 at 12 44 38 PM

New
Screen Shot 2019-10-09 at 12 43 15 PM

follow-up to: #5997
Issue: #5937

@miq-bot miq-bot added the wip label Oct 9, 2019
@epwinchell epwinchell mentioned this pull request Oct 9, 2019
Closed
@epwinchell
Copy link
Contributor Author

@miq-bot add_label formatting/styling, ivanchuk/no

@epwinchell
Copy link
Contributor Author

@miq-bot add_reviewer @skateman

@epwinchell epwinchell changed the title [WIP] Toolbar Simplification Toolbar Simplification Oct 9, 2019
@miq-bot miq-bot requested a review from skateman October 9, 2019 16:40
@epwinchell
Copy link
Contributor Author

@miq-bot assign @martinpovolny

@miq-bot miq-bot removed the wip label Oct 9, 2019
skateman
skateman reviewed Oct 11, 2019
View reviewed changes
app/helpers/application_helper/toolbar/ansible_credential_center.rb Outdated
@@ -2,7 +2,7 @@ class ApplicationHelper::Toolbar::AnsibleCredentialCenter < ApplicationHelper::T
button_group('ansible_repository', [
select(
:ansible_credential_configuration,
'fa fa-cog fa-lg',
'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather go with nil instead of allocating an empty string each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skateman Updated

@epwinchell epwinchell requested a review from skateman October 11, 2019 22:58
@miq-bot
Copy link
Member

miq-bot commented Oct 11, 2019

Checked commits https://github.com/epwinchell/manageiq-ui-classic/compare/28ae9ba3a50c24802eb70bab6a2243fb9d7a3119~...36ef4978a7e413666db87af72f0dd143a8b61c65 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
221 files checked, 0 offenses detected
Everything looks fine. ⭐

@martinpovolny martinpovolny merged commit 10ab66b into ManageIQ:master Oct 12, 2019
@martinpovolny martinpovolny added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 12, 2019
@himdel
Copy link
Contributor

himdel commented Oct 14, 2019

@epwinchell Didn't quite catch all of them it seems ;)

provider

(I disagree with this in #5937 (comment), but if we're going that way, I guess we should remove all of them?)

@himdel
Copy link
Contributor

himdel commented Oct 14, 2019 •
edited
Loading

This should help finding the rest (not claiming all of those are relevant, this is only a list of all icons mentioned at the right indent level after a select():

ems_container_center.rb
115:    select(
117:      'fa fa-lock fa-lg',

cloud_tenant_center.rb
23:    select(
40:      'fa fa-tachometer fa-1xplus',
49:      'fa fa-th-list',

ems_cloud_center.rb
89:    select(
91:      'fa fa-lock fa-lg',
108:      'fa fa-tachometer fa-1xplus',
117:      'fa fa-th-list',

ems_physical_infras_center.rb
96:    select(
98:      'fa fa-lock fa-lg',

ems_containers_center.rb
102:    select(
104:      'fa fa-lock fa-lg',

gtl_view.rb
29:    select(
31:      'fa fa-download fa-lg',

miq_capacity_view.rb
3:    select(
5:      'fa fa-download fa-lg',

template_clouds_center.rb
92:    select(
94:      'pficon pficon-add-circle-o fa-lg',

ems_physical_infra_center.rb
81:    select(
83:      'pficon pficon-screen fa-lg',
101:    select(
103:      'fa fa-lock fa-lg',

ems_clouds_center.rb
90:    select(
92:      'fa fa-lock fa-lg',

diagnostics_zone_center.rb
68:    select(
70:      'fa fa-filter fa-lg',

compare_view.rb
19:    select(
21:      'fa fa-download fa-lg',

ems_infras_center.rb
98:    select(
100:      'fa fa-lock fa-lg',

host_center.rb
130:    select(
132:      'fa fa-power-off fa-lg',

tasks_center.rb
12:    select(
14:      'pficon pficon-delete fa-lg',

diagnostics_server_center.rb
60:    select(
62:      'fa fa-filter fa-lg',

ems_infra_center.rb
102:    select(
104:      'fa fa-lock fa-lg',
119:    select(
121:      'pficon pficon-screen fa-lg',

drift_view.rb
19:    select(
21:      'fa fa-download fa-lg',

report_view.rb
37:    select(
39:      'fa fa-download fa-lg',

chargeback_center.rb
3:    select(
5:      'fa fa-download fa-lg',

x_vm_snapshot_center.rb
10:    select(
12:      'pficon pficon-delete fa-lg',

x_gtl_view.rb
29:    select(
31:      'fa fa-download fa-lg',

(courtesy of ag '^(( {4}(button|select))| {6}["'\''])' plus some filtering)

@skateman
Copy link
Member

@himdel the download button intentionally has an icon as it doesn't have text, the rest should go.

@himdel
Copy link
Contributor

himdel commented Oct 14, 2019 •
edited
Loading

My last comment was under the assumption that we only want to remove icons from first-level dropdowns.
There will be more if this is supposed to also affect actual first-level buttons (like "Refresh").

@epwinchell
Copy link
Contributor Author

My last comment was under the assumption that we only want to remove icons from first-level dropdowns.

@himdel We're only dealing with first-level dropdowns for this issue.

@epwinchell epwinchell mentioned this pull request Oct 15, 2019
Merged
@himdel himdel mentioned this pull request Oct 16, 2019
Merged
@epwinchell epwinchell mentioned this pull request Oct 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skateman skateman Awaiting requested review from skateman

Assignees

@martinpovolny martinpovolny

Labels
formatting/styling ivanchuk/no
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@epwinchell @miq-bot @himdel @skateman @martinpovolny